I used alpine image because it's tiny (2mb) and should provide a consistent test no matter what the user has done to their build image. It's far-fetched, but the build image might not have ls or it might have a different default output format. I think it's best to avoid any such issues by using a small known test-case.

Hi @kichik , is it alright if we revert this since this is trying way too hard just to support a legacy deprecated old version called Docker Toolbox for Windows?

As far as I can remember, the previous solution already supports both Cygwin and WSL and many other permutations, as long as the recommended version of Docker for Windows is installed. There was much careful thought and consideration put into those RegExp .

@heri16 please don't. It's not always possible to install Docker for Windows. Some business use cases still require VirtualBox or VMware and Docker for Windows disables those. And using WSL or Cygwin is not always user-friendly enough.

I am sorry @kichik, let's revert this change and then we will come up with a better solution together in another PR. Likely doing a bindPath check on the modern path then fallback to some legacy Paths if it does not work out. Please indicate what are the paths you see on legacy docker toolbox. @dschep

@dschep agree to move forward to revert this change?

@heri16 why would you want to revert something that works?

And if you're already doing a fallback in case it fails (which I assume is going to be tested with tryBindPath() or something similar), why not make accommodation for all cases?

Because using alpine and blindly looping through paths is exactly what I did during my first try at this. And it was ugly and an obvious quick hack.

We are going in circles. I'll leave the decision with @dschep. He's the one who's going to have to handle the bug reports for either solution we pick.

To summarize, my technical reasons for this patch are:

• Previous solution didn't work with Docker Toolbox (unless you used WSL/Cygwin and IIRC I had problems with WSL too but I might be remembering this one incorrectly)
• Docker Toolbox might be legacy but is still required for a lot of business use cases and is still being updated by Docker
• WSL/Cygwin requires you to change your whole dev environment
• Previous solution required intimate knowledge of Docker versions and different Windows shells (detecting all the following with heuristics - Docker Toolbox, Docker for Windows, Cygwin, WSL)
• Previous solution didn't support gitbash or any other solution that had lower case C: in the mount name (like gitbash where it's /c/Users/... but Docker Toolbox expects it to be upper case)
• The new trial & error solution should support more combinations of Docker/shell, will not fail as heuristic detection sources change, will not fail with custom shells that do things a little different, and is therefore a bit more future-proof
• Sometimes brute-force is the simpler solution because dealing with all the corner-cases is too complicated

@kichik would it be possible to implement docker toolbox support as a flag instead of detecting it by actually running docker with mounts? If so, I think that's my preferred path forward. No docker trial&error, and the Docker Toolbox workflow is still supported, albeit not 100% automatically.

I wouldn't call it user friendly, but it could work.

We are basically moving the detection responsibility from us to the build system that invokes us.

Is the iterative process absolutely necessary or can the right path be determined if enough information is available? If it's the later, I'd rather that. We can detect WSL, is tehre a way to detect gitbash, cygwin and toolbox vs 4windows? (for the later, maybe if toolbox uses tcp:// and 4windows doesnt? https://docs.docker.com/docker-for-windows/faqs/#how-do-i-connect-to-the-remote-docker-engine-api)

You can theoretically gather all the right information and build the right path. There are three issues I can see with that:

1. Collecting this information requires heuristics and is mostly based on undocumented methods. Checking tcp:// is not guaranteed to work over time. The one I was considering when writing this patch was to keep using docker version like we do now, but check the experimental flag in combination with architecture. I believe it was linux architecture and experimental true for Docker for Windows. That's not a strong detection and can easily change and break.
2. We can theoretically find proper detection methods for everything that are somewhat documented. For example MSYSTEM="MINGW64" might be used to detect gitbash. We will keep having to tune these detection methods and support more and more combinations and configurations.
3. Using a set list of detection methods and one set bind path result for each will mean we won't automatically support minor changes and won't be able to support new combinations that come up. It's not going to be a huge headache, but won't it be cool if it just works everywhere?

So to answer your question, it is not absolutely necessary. It's just the easier solution that will make this easier to use. From my experience with Windows configurations, this is sometimes the best solution.

Like I said, both sides have their merits. You always want to depend on faster heuristics first, then fallback on a slower method if it does not work. Please don't go about going to other open source projects and removing gcc compile time optimizations, platform detection macros, and time honored bloom filters, just because you don't understand what they are doing. Just take a look at how pollyfills and browser shims work in the javascript ecosystem, they always check the environment, then load in the pollyfill only when required.

In open source community, it is only polite to give the PR title "replace or change" when you are radically changing a previous implementation. If you say "add support", then please add. For us to have a more welcoming community, we need to understand the time honored unspoken rules of the open source movement.

We need a new PR that merges both approaches. I like both (just because I understand how finicky windows can be sometimes).

I was worried about this slowing things too when coding it. I originally ran some tests and found it runs in well under a second and has a negligible effect on the total time. Let me know if you have different results.

That said, we can combine both methods. I'd just like to ask we don't temporarily disable existing features by reverting anything. It will probably speed it up a bit to optimize this with some heuristics if we still test to make sure it worked and fallback to some common paths.

@heri16 I'd appreciate it if we can keep this discussion professional. I'm happy to discuss facts and technical details regarding this code, but am uninterested in discussing other matters.

Could you please provide documentation on how you arrived at these set of common paths? I have rechecked official specs written by docker and msdn that were used to create the original approach, and feel that the only additional case you need is gitbash.

Can I confirm that you are not using Wsl? You are using legacy docker toolbox? What is your terminal?

WSL Related: https://blogs.msdn.microsoft.com/commandline/2017/12/08/cross-post-wsl-interoperability-with-docker/

Could you also provide the output on your environment when running "docker version"?

Docker Toolbox Related:

• https://stackoverflow.com/questions/34161352/docker-sharing-a-volume-on-windows-with-docker-toolbox

• https://superuser.com/questions/1051520/docker-windows-container-how-to-mount-a-host-folder-as-data-volume-on-windows

• https://stackoverflow.com/questions/33312662/docker-toolbox-mount-file-on-windows

• https://stackoverflow.com/questions/33126271/how-to-use-volume-option-with-docker-toolbox-on-windows

Findings:

• No need for uppercase drive letters.
• Use windows style semantics (c:/) when using new windows docker-cli?
• Use //c/ when using windows docker-toolbox-cli?

Sorry I haven't been too involved, I don't use windows ;)

I think hacks are ok if they make user experience easier and more reliable so I'm inclined to leave this. @heri16 have you noticed any performance issue because of this (i guess not unless you've added timing around this function)

I have confirmed that there at most two paths that needs to be checked.

• /c/ style
• c:/ style

There is no such requirement for drive.toUppercase() according to all official docs. Neither is the case of /mnt/c/ ever valid.

I have found that the docker-cli API on windows does not change sporadically. The only change between legacy-docker and new docker has been documented clearly. In fact, once we read docker's architecture and documentation, it gives a lot of guarantees on what kind of paths it is expecting for the docker-cli.

As such I am now very sure that this is very easily and simply solved by just checking the output of "docker version", and catering for the legacy version numbers.

Currently of the opinion that we don't need a tryBindPath fallback because asking 'docker version' is deterministic.

I am not using WSL, just normal cmd.exe. For me the bind path should be /c/Users/.....

Based on your findings, we should at the very least move /c and c:/ to be tested first.

It's been a while, so honestly I don't entirely remember how I arrived at all the rest of the paths. I should have added a comment for each in the code. They may have very well been "just in case" as my original comment here suggests. The idea being it should cover all conceivable configurations (custom shared folders in docker-machine, weird setups, future changes, and others).

It also tries really hard to find a working mount path no matter the platform combination. This should hopefully handle any corner case that we can ever come across.

I understand you believe we shouldn't do any work we don't 100% need to do and only fix issues after users bring them up. You know my position. You have not shared performance metrics, so I assume you agree the bind path loop is negligible. I suggest we leave the decision about this part in the hands of @dschep and not discuss this part further.

Now, as for automatically detecting Docker Toolbox... This is my docker version. As you can see, there is nothing specific there that says it's running Docker Toolbox. Let me know if you found a detection method that you believe will last.

Client:
 Version:       18.02.0-ce
 API version:   1.36
 Go version:    go1.9.4
 Git commit:    fc4de447b5
 Built: Mon Feb 12 19:03:38 2018
 OS/Arch:       windows/amd64
 Experimental:  false
 Orchestrator:  swarm

Server:
 Engine:
  Version:      18.05.0-ce
  API version:  1.37 (minimum version 1.12)
  Go version:   go1.10.1
  Git commit:   f150324
  Built:        Wed May  9 22:20:42 2018
  OS/Arch:      linux/amd64
  Experimental: false

Did I miss anything?

Thanks @kichik

@kichik would it be alright if you test that the version without toUppercase() and without /mnt works on your Docker Toolbox environment?

Reference Docs: https://docs.docker.com/machine/overview/#whats-the-difference-between-docker-engine-and-docker-machine

Yes, it works for my case without uppercase and /mnt.

I am not sure why you posted that reference doc. Is that a response to my question about telling Docker Toolbox and Docker for Windows apart? I don't see that information in there.